Skip to content

Evict larger limits first using spill weights#249

Merged
cfallin merged 1 commit intobytecodealliance:mainfrom
abrown:evict-larger-limits
Jan 9, 2026
Merged

Evict larger limits first using spill weights#249
cfallin merged 1 commit intobytecodealliance:mainfrom
abrown:evict-larger-limits

Conversation

@abrown
Copy link
Member

@abrown abrown commented Dec 13, 2025

Consider an instruction that could potentially overallocate to a limited range of registers:

Use: v0i limit(0..=1), Use: v0i limit(0..=1), Use: v1i limit(0..=7)

Both uses of v0 must fit in registers 0-1 and v1 should find its home somewhere else in registers 2-7. But fuzzing found a case where this failed. If v1 was allocated first--say to register 0--and one of the uses of v0 allocated to the remaining register--register 1--the final use of v0 had nowhere to go and could not evict either of the previous allocations. This caused ion to fail with TooManyLiveRegs when in fact a solution was possible.

(Why are the identical uses of v0 allocated separately? Can't they use the same register? I'm not entirely sure, but I think ion had split things down to minimal bundles).

The fix in this change is to alter the default spill weights assigned to a minimal bundle. Previously, minimal bundles with a fixed constraint received the highest weight, followed by other minimal bundles and then non-minimal bundles. This reserves weights for limits into that order:

  • minimal with fixed constraint
  • minimal with limit(0..=0) constraint
  • minimal with limit(0..=1) constraint
  • ...
  • minimal with limit(0..=255) constraint
  • minimal, any other constraint
  • non-minimal

Doing this allows ion to evict bundles with higher limits (e.g., v1i limit(0..=7) once they become minimal, allowing allocation to continue.

@abrown @cfallin @rahulchaphalkar
Evict larger limits first using spill weights
317dc20 
Consider an instruction that could potentially overallocate to a limited
range of registers:

```
Use: v0i limit(0..=1), Use: v0i limit(0..=1), Use: v1i limit(0..=7)
```

Both uses of `v0` _must_ fit in registers 0-1 and `v1` should find its
home somewhere else in registers 2-7. But fuzzing found a case where
this failed. If `v1` was allocated first--say to register 0--and one of
the uses of `v0` allocated to the remaining register--register 1--the
final use of `v0` had nowhere to go _and_ could not evict either of the
previous allocations. This caused `ion` to fail with `TooManyLiveRegs`
when in fact a solution was possible.

(Why are the identical uses of `v0` allocated separately? Can't they use
the same register? I'm not entirely sure, but I think `ion` had split
things down to minimal bundles).

The fix in this change is to alter the default spill weights assigned to
a minimal bundle. Previously, minimal bundles with a fixed constraint
received the highest weight, followed by other minimal bundles and then
non-minimal bundles. This reserves weights for limits into that order:
- minimal with `fixed` constraint
- minimal with `limit(0..=0)` constraint
- minimal with `limit(0..=1)` constraint
- ...
- minimal with `limit(0..=255)` constraint
- minimal, any other constraint
- non-minimal

Doing this allows `ion` to evict bundles with higher  limits (e.g., `v1i
limit(0..=7)` once they become minimal, allowing allocation to continue.

Co-authored-by: Chris Fallin <chris@cfallin.org>
Co-authored-by: Rahul Chaphalkar <rahul.s.chaphalkar@intel.com>
@abrown abrown marked this pull request as draft December 13, 2025 01:07
@abrown
Copy link
Member Author

abrown commented Dec 13, 2025

No hurry to merge this just yet; I'm running this in my branch that enables fuzzing of limits: fuzz-limit-constraints-rebased. Thanks to @rahulchaphalkar for helping discover a minimizable test case and @cfallin for talking through a solution!

@abrown
Copy link
Member Author

abrown commented Dec 13, 2025 •
edited
Loading

Also, if it is helpful, I can include an extra commit here with a unit test that runs the exact test case that caused the original problem:

{
  block0(): # succs:[] preds:[]
    inst0: Op ops:[Def@Early: v0i limit(0..=3)] clobber:[]
    inst1: Op ops:[Def: v1i fixed(p25i), Use: v0i fixed(p26i), Use: v0i fixed(p27i)] clobber:[]
    inst2: Op ops:[Def: v2i reg, Use: v0i limit(0..=1), Use: v0i limit(0..=1), Use: v1i limit(0..=7)] clobber:[]
    inst3: Op ops:[Def: v3i reg, Use: v0i fixed(p28i), Use: v0i any] clobber:[]
    inst4: Ret ops:[] clobber:[]
}

cfallin
cfallin approved these changes Dec 16, 2025
View reviewed changes
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (no surprise since we pair-programmed this fix) -- thanks a bunch for working on this!

Regarding the unit test for this case -- I guess it depends on whether the fuzzer would cover this (ie the reason we don't have a test suite otherwise, since the fuzzer coverage is so good). This case was originally discovered via fuzzing and the original fuzzbug is now fixed? If so then I think it's fine; otherwise it might be good to throw in an explicit test. Thanks!

@cfallin
Copy link
Member

cfallin commented Jan 9, 2026

It's been a bit and this change still looks right to me, so I'm going to go ahead and take this out of draft mode and merge. Thanks again!

@cfallin cfallin marked this pull request as ready for review January 9, 2026 09:20
@cfallin cfallin merged commit 600c63e into bytecodealliance:main Jan 9, 2026
6 checks passed
cfallin added a commit to cfallin/regalloc2 that referenced this pull request Jan 9, 2026
@cfallin
Release 0.13.5.
e079b8e 
Includes bytecodealliance#249.
@cfallin cfallin mentioned this pull request Jan 9, 2026
Merged
cfallin added a commit that referenced this pull request Jan 9, 2026
@cfallin
Release 0.13.5. (#253)
33c6242 
Includes #249.
@abrown
Copy link
Member Author

abrown commented Jan 9, 2026 •
edited
Loading

@cfallin, apologies for not following up with the fuzzing results. I did run fuzzing for this change using fuzz-limit-constraints-rebased and found a fuzz bug that was unrelated to this change. IIRC, the test case did not even have limit constraints at all and my conclusion was that the changes in that branch allowed the fixed constraints to get a little bit too... constrained, causing the "too many live regs" failure. Looking at that derailed me a bit (plus Christmas, jobs, etc.!) so, in conclusion, I would (1) agree that this is still the right change, even if we couldn't properly fuzz it, and (2) alert @rahulchaphalkar that fuzz-limit-constraints-rebased might need some extra eyes on whatever changed with fixed registers (I'll send you the test case).

@abrown abrown deleted the evict-larger-limits branch January 9, 2026 16:35
@abrown
Copy link
Member Author

abrown commented Jan 9, 2026

After looking around I cannot find the fixed-only crash I just mentioned but here is one that fails on the interaction between limit and fixed; I still think the handling of fixed on that branch could use another look:

{
      block0(): # succs:[] preds:[]
        inst0: Op ops:[Def: v0i reg] clobber:[]
        inst1: Op ops:[Def@Early: v1i reg, Use: v0i any, Use: v0i fixed(p35i)] clobber:[]
        inst2: Op ops:[Def@Early: v2i fixed(p0i), Use: v0i any, Use: v0i fixed(p9i), Use: v0i any, Use: v0i any, Use: v0i any, Use: v0i any] clobber:[]
        inst3: Op ops:[Def: v3i limit(0..=7), Use: v0i reg, Use: v0i any, Use: v0i reg, Use: v0i limit(0..=7), Use: v2i limit(0..=7), Use: v2i limit(0..=7), Use: v2i limit(0..=7), Use: v2i limit(0..=15), Use: v2i limit(0..=15)] clobber:[]
        inst4: Op ops:[Def@Early: v4i limit(0..=7), Use: v0i limit(0..=7), Use: v0i limit(0..=7), Use: v0i limit(0..=7), Use: v0i limit(0..=7), Use: v0i limit(0..=7), Use: v0i limit(0..=7), Use: v0i limit(0..=7), Use: v2i limit(0..=15), Use: v1i fixed(p0i)] clobber:[]
        inst5: Op ops:[Def@Early: v5i any] clobber:[]
        inst6: Op ops:[Def@Early: v6i any] clobber:[]
        inst7: Op ops:[Def@Early: v7f any] clobber:[]
        inst8: Op ops:[Def@Early: v8i any] clobber:[]
        inst9: Op ops:[Def@Early: v9v any] clobber:[]
        inst10: Op ops:[Def@Early: v10i any] clobber:[]
        inst11: Op ops:[Def@Early: v11i any] clobber:[]
        inst12: Ret ops:[] clobber:[]
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cfallin cfallin cfallin approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@abrown @cfallin